Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

♿️ Restore platform-centric menu names for showing in file manager #1311

Merged
merged 2 commits into from Apr 18, 2019

Conversation

mattlubner
Copy link
Contributor

Description of the Change

Addresses #1307. Restore platform-centric menu names for showing files in the user's file manager.

Benefits

  • Following each platform's idiomatic naming conventions will improve usability.

Possible Drawbacks

  • The name of the platform's default file manager is always used. This will be slightly misleading if shell.showItemInFolder opens a third-party file manager.

Applicable Issues

@mattlubner
Copy link
Contributor Author

Hey @rsese @50Wliu, how do we proceed? Should I attempt to build Atom locally, write a test, or do something else?

This is the first time I've written any Coffeescript (or contributed to Atom), so any guidance / input is appreciated! 🙏

@@ -560,13 +560,22 @@ class TreeView
return unless filePath = @selectedEntry()?.getPath()

unless shell.showItemInFolder(filePath)
atom.notifications.addWarning("Unable to show #{filePath} in file manager")
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName().toLowerCase()}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably leave the case unmodified here.

Suggested change
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName().toLowerCase()}")
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName()}")


showCurrentFileInFileManager: ->
return unless filePath = atom.workspace.getCenter().getActiveTextEditor()?.getPath()

unless shell.showItemInFolder(filePath)
atom.notifications.addWarning("Unable to show #{filePath} in file manager")
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName().toLowerCase()}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably leave the case unmodified here.

Suggested change
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName().toLowerCase()}")
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName()}")

@rsese
Copy link

rsese commented Apr 17, 2019

Thanks @mattlubner!

Should I attempt to build Atom locally, write a test, or do something else?

No need to build Atom locally, you can follow this guide to link your modified version of tree view:

https://flight-manual.atom.io/hacking-atom/sections/contributing-to-official-atom-packages/

I confirmed that tests aren't necessary in this case but it would be good if you can manual verify that things look ok on Windows, macOS, and Linux with your changes.

@mattlubner
Copy link
Contributor Author

mattlubner commented Apr 17, 2019

Thanks for the review @lee-dohm and the feedback @rsese! Just manually verified that things look good on macOS. I don't have a Window or Linux environment that I can test on, though.

@50Wliu 50Wliu merged commit 9b2d459 into atom:master Apr 18, 2019
@50Wliu
Copy link
Contributor

50Wliu commented Apr 18, 2019

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants